Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Batch of fixes 20141231 #170

Closed

Conversation

aarononeal
Copy link
Contributor

The last commit is dependent on some of the former so I'm batching all these together. I added detailed descriptions to each commit to make review easier.

Two of the more significant issues fixed include binary string truncation and blob support.

The plugin id used here must match the one in plugin.xml or the test runner will fail to remove it when deployed.
@brodycj
Copy link
Contributor

brodycj commented Jan 1, 2015

Happy New Year! Many thanks for your efforts to fix the support for Base64,
return types, and other issues. I am going on a short vacation and will
look into integrating the fixes sometime next week.

I do really like the cleanup of the Base64 code. I suspect it triggers a
dependency on a more recent version of Cordova and will label it, and
describe a workaround for those who are using an older version of Cordova.

I am also wondering what will happen if we try the Base64 on WP8. If it
doesn't work we can simply skip the Base64 test on WP8.

Chris

On Thursday, January 1, 2015, Aaron Oneal notifications@github.com wrote:

The last commit is dependent on some of the former so I'm batching all
these together. I added detailed descriptions to each commit to make review
easier.

Two of the more significant issues fixed include binary string truncation

and blob support.

You can merge this Pull Request by running

git pull https://github.com/spicypixel/cordova-plugin-websql hotfix/batch-20141231

Or view, comment on, or merge it at:

#170
Commit Summary

  • Fix test runner to use correct plugin id.
  • Fix database close and reopen test.
  • Propagate statement failures to transaction failures.
  • Fix error types on open, close, and deleteDb.
  • Fix transaction and statement errors to conform to SQLError.
  • Fix to prevent double marshaling of data.
  • Fix iOS regex args check.
  • Fix truncation in iOS query result string encoding.
  • Fix warning regarding test runner viewport format.
  • Support SQL blob marshaling using ArrayBuffer and Base64.

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#170.

Sent from my mobile

The test was closing the database in the middle of an execute transaction. This would cause the transaction to fail because at the exit of the transaction handler a commit would be issued on a closed database. This fix moves the close to the success handler of the transaction so that the final close occurs after the transaction ends.

Additionally, the db variable is now obtained from the open callback instead of the function result, which is a more consistent pattern for async.

Finally, the close method was updated to fail if an attempt is made to call it while a transaction is active and a test was added to confirm this behavior.
According to the W3C spec, if a transaction failure callback *does not return false* then the error is propagated to a transaction failure. This means that if a transaction returns true, but more importantly, if it has no return value (undefined) then the error must propagate to the transaction. This fix explicitly requires false to correct the propagation.

A subsequent fix will address the fact that the original statement error is not propagated and is replaced here, but that is unrelated to this issue.
The aforementioned methods receive errors as string messages from the native layer, so these strings must be wrapped in a regular `Error` type before passing on to API callbacks.

This fix also makes 2 minor changes to open:
1) remove a semicolon per doc convention
2) only call the success callback if there is one (in case the default handler that logs is ever removed)
@aarononeal
Copy link
Contributor Author

Happy New Year! :-)

It looks like the NSData+Base64 files have been around in one shape or another since early 2012, but I only tested against the latest Cordova build.

On WP8, what I expect will happen is the ArrayBuffer will encode to a new sqlblob URI and get serialized by the "native" side as text because it doesn't have logic yet to recognize the prefix. When read back, the sqlblob will just read as text and the JS side will be able to decode it. So, the new tests should pass and the only real difference will be that the backing data will be stored as a string instead of binary. Still unsupported in WP8 is the ability to read a binary blob from an externally created database back as anything other than text. We need to have it return a sqlblob in those cases like Android and iOS do now. At some point I'll be able to test on WP8.

Also, I made 2 minor updates to the pull tonight. I updated the close commit 8c3bdcc so that the close method verifies it's not in a transaction. This would have avoided the original test bug had it been in place. I also added commit 96d5db8 so that executeSql throws when it's supposed to in order to make it more obvious why the plugin fails when used with ES6 promises.

@nolanlawson
Copy link
Contributor

Very cool, and the new tests are highly appreciated.

@aarononeal aarononeal force-pushed the hotfix/batch-20141231 branch 8 times, most recently from 33bbfa2 to f95f5d5 Compare January 6, 2015 02:59
Sometimes the native layer returns errors as {message:string, code:number} and other times as just a string. This fix wraps key places to ensure that errors returned through the API always conform to SQLError per the API contract documented in the W3C spec.

In short, errors consistently have a message and code now and propagate from statement failure up to the transaction.
Moving data between JavaScript and native is extremely slow, especially for binary which Cordova has to base64 encode and then transmit over XHR or as an URL parameter. The last thing we want to do is send more data. For some reason SQL statements and their parameters were being sent twice because they were duplicated to a `query` member. That member was only used by the iOS plugin, so this fix removes it and updates the iOS plugin to use the sql and params members like the others.

Additionally, the iOS plugin is updated to handle unsupported types during binding and a test was added to verify.
The regex function for iOS was reading arguments before checking that the correct number were specified.
This fix enables end-to-end encoding of binary data using strings.

Strings with Unicode values like `\u0000` were being truncated because the string constructor was using a null terminator to determine size instead of the stored byte length. The fix uses a different constructor and the stored byte length so that the full length string is returned.
If executeSql is called on a finalized transaction it should throw. If it does not, timing errors get a lot harder to debug. ES6 promises, for example, generally execute on the next tick and then it becomes unclear why a statement fails to execute.
Web SQL doesn't actually support storing binary data in SQL blobs using the SQLite blob methods. It serializes binary data using strings with the SQLite text methods which results in data not being stored as a blob. This can be problematic in Cordova if you need to work with existing SQLite databases.

This change adds more robust support for binary serialization. `SQLBlob` objects can be used in statements and these will be converted to a `sqlblob` URI with base64 or URL encoded data that is unpacked on the native side and stored as a proper binary blob.

When reading a blob back from the native side, the previous behavior was to return the blob as base64. Now, the blob can be decoded as a `SQLBlob` object which can then be unpacked in JavaScript to base64, ArrayBuffer, or binary string.

  tx.executeSql('INSERT INTO test_table VALUES (?)', [new SQLBlob(arrayBuffer)]);

  tx.executeSql("SELECT foo FROM test_table", [], function(tx, res) {
    var blob = new SQLBlob(res.rows.item(0).foo);
    blob.toBase64();
    blob.toBinaryString();
    blob.toArrayBuffer();
  });

Tests were added to verify the behavior and to demonstrate usage.

Only iOS and Android were updated. Windows Phone did not previously have blob support; saving for a future update.
brodycj pushed a commit that referenced this pull request Feb 6, 2015
Moving data between JavaScript and native is extremely slow, especially for binary which Cordova has to base64 encode and then transmit over XHR or as an URL parameter. The last thing we want to do is send more data. For some reason SQL statements and their parameters were being sent twice because they were duplicated to a `query` member. That member was only used by the iOS plugin, so this fix removes it and updates the iOS plugin to use the sql and params members like the others.

Additionally, the iOS plugin is updated to handle unsupported types during binding and a test was added to verify.

(@brodybits) from pull #170, adding www/SQLitePlugin.js updated from SQLitePlugin.coffee.md
brodycj pushed a commit that referenced this pull request Feb 6, 2015
The plugin id used here must match the one in plugin.xml or the test runner will fail to remove it when deployed.

(@brodybits) from pull request #170 thanks @aarononeal
brodycj pushed a commit that referenced this pull request Feb 6, 2015
The test was closing the database in the middle of an execute transaction. This would cause the transaction to fail because at the exit of the transaction handler a commit would be issued on a closed database. This fix moves the close to the success handler of the transaction so that the final close occurs after the transaction ends.

Additionally, the db variable is now obtained from the open callback instead of the function result, which is a more consistent pattern for async.

Finally, the close method was updated to fail if an attempt is made to call it while a transaction is active and a test was added to confirm this behavior.

(@brodybits) from pull #170 thanks @aarononeal, adding www/SQLitePlugin.js as updated from SQLitePlugin.coffee.md
@brodycj
Copy link
Contributor

brodycj commented Feb 6, 2015

Thanks @aarononeal, I will include the changes using git cherry-pick due to the branching model of this project.

My only major comment is that if you propose more changes in the future, changes to the Javascript need to be included when we change the CoffeeScript for the Cordova CLI installation to work. I will take care of this for the changes that you have already proposed.

@aarononeal
Copy link
Contributor Author

Thanks. I wasn't sure how you wanted to handle integration so I left out the auto-generated JS, but I can definitely include that in the future.

brodycj pushed a commit that referenced this pull request Feb 18, 2015
Important changes:
- Implement pre-population ref: #10/#172
- Choose DB location to make iCloud backup optional ref: #16/#143
- Fix db close conditions & prevent double-marshaling of data ref: pull #170
@brodycj brodycj mentioned this pull request Feb 25, 2015
brodycj pushed a commit that referenced this pull request Feb 28, 2015
According to the W3C spec, if a transaction failure callback *does not return false* then the error is propagated to a transaction failure. This means that if a transaction returns true, but more importantly, if it has no return value (undefined) then the error must propagate to the transaction. This fix explicitly requires false to correct the propagation.

A subsequent fix will address the fact that the original statement error is not propagated and is replaced here, but that is unrelated to this issue.

(@brodybits) from PR #170 thanks @aarononeal, adding www/SQLitePlugin.js as updated from SQLitePlugin.coffee.md
brodycj pushed a commit that referenced this pull request Feb 28, 2015
Sometimes the native layer returns errors as {message:string, code:number} and other times as just a string. This fix wraps key places to ensure that errors returned through the API always conform to SQLError per the API contract documented in the W3C spec.

In short, errors consistently have a message and code now and propagate from statement failure up to the transaction.

(@brodybits) from PR #170 thanks @aarononeal, adding www/SQLitePlugin.js as updated from SQLitePlugin.coffee.md
brodycj pushed a commit that referenced this pull request Feb 28, 2015
brodycj pushed a commit that referenced this pull request Feb 28, 2015
See PR #170: similar to commit c19698b but with a different solution
brodycj pushed a commit that referenced this pull request Feb 28, 2015
Parameters use `,` not `;`.

(@brodybits) from PR #170 thanks @aarononeal
brodycj pushed a commit that referenced this pull request Mar 1, 2015
If executeSql is called on a finalized transaction it should throw. If it does not, timing errors get a lot harder to debug. ES6 promises, for example, generally execute on the next tick and then it becomes unclear why a statement fails to execute.

(@brodybits) from PR #170 thanks @aarononeal, adding www/SQLitePlugin.js as updated from SQLitePlugin.coffee.md
brodycj pushed a commit that referenced this pull request Mar 1, 2015
Strings with Unicode values like `\u0000` ARE being truncated because the string constructor was using a null terminator to determine size instead of the stored byte length.

(@brodybits) from PR #170 thanks @aarononeal. The fix will be committed separately.
brodycj pushed a commit that referenced this pull request Mar 1, 2015
This fix enables end-to-end encoding of binary data using strings.

Strings with Unicode values like `\u0000` were being truncated because the string constructor was using a null terminator to determine size instead of the stored byte length. The fix uses a different constructor and the stored byte length so that the full length string is returned.

(@brodybits) from PR #170 thanks @aarononeal. Test was already committed separately.
brodycj pushed a commit that referenced this pull request Mar 4, 2015
… for improvements to SQL blob storage]

(@brodybits) from PR #170 thanks @aarononeal.

NOTE: changes related to SQL blob binding are #ifdef'd out (TBD subjet to change).
brodycj pushed a commit that referenced this pull request Mar 4, 2015
…onditionally-included SQL binding code (ref: PR #170)
brodycj pushed a commit that referenced this pull request Mar 6, 2015
Small fix to handling of Blob to prepare for SQL blob improvements (ref: PR #170)
brodycj pushed a commit that referenced this pull request Mar 10, 2015
Fixes:
- #193: workaround for Android db locking/closing issue
- #144: convert array parameters to string to match Web SQL
- #199: fix double-precision REAL values in result for iOS version
- #150/#153: close Android db before removing from map
- Fix truncation in iOS query result in case of UNICODE NULL (\0 or \u0000) (ref: PR #170)
- Some fixes for error handling to be consistent with Web SQL (ref: PR #170)

Testing ONLY:
- #147: testing with UNICODE line separator
- #195: Reproduce issue with double-precision REAL number on WP(8) ONLY
brodycj pushed a commit that referenced this pull request Mar 10, 2015
Fixes:
- #193: workaround for Android db locking/closing issue
- #144: convert array parameters to string to match Web SQL
- #199: fix double-precision REAL values in result for iOS version
- #150/#153: close Android db before removing from map
- Fix truncation in iOS query result in case of UNICODE NULL (\0 or \u0000) (ref: PR #170)
- Some fixes for error handling to be consistent with Web SQL (ref: PR #170)

Testing ONLY:
- #147: testing with UNICODE line separator
- #195: Reproduce issue with double-precision REAL number on WP(8) ONLY
brodycj pushed a commit that referenced this pull request Mar 10, 2015
Related to the following fixes:
- #193: workaround for Android db locking/closing issue
- #144: convert array parameters to string to match Web SQL
- #199: fix double-precision REAL values in result for iOS version
- #150/#153: close Android db before removing from map
- Fix truncation in iOS query result in case of UNICODE NULL (\0 or \u0000) (ref: PR #170)
- Some fixes for error handling to be consistent with Web SQL (ref: PR #170)
brodycj pushed a commit that referenced this pull request Mar 12, 2015
@brodycj brodycj closed this Apr 2, 2015
@brodycj
Copy link
Contributor

brodycj commented Apr 22, 2016

For general reference: all changes in this PR are included except for the BLOB enhancements. I plan to integrate the blob enhancements with the cordova-sqlite-ext version when I get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants